Hello @yanglinlee! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-06-01 00:32:57 UTC

Codecov Report

Merging #26514 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #26514      +/-   ##
==========================================
- Coverage   91.76%   91.75%   -0.01%     
==========================================
  Files         174      174              
  Lines       50679    50684       +5     
==========================================
  Hits        46504    46504              
- Misses       4175     4180       +5

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b730ab3...cf156cf. Read the comment docs.

Codecov Report

Merging #26514 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #26514      +/-   ##
==========================================
- Coverage   91.84%   91.83%   -0.01%     
==========================================
  Files         174      174              
  Lines       50644    50647       +3     
==========================================
- Hits        46515    46514       -1     
- Misses       4129     4133       +4

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c6a7cc1...d83c4f4. Read the comment docs.

@yanglinlee : Thanks for submitting this! Can you also add a whatsnew entry to v0.25.0.rst?

@yanglinlee : Thanks for submitting this! Can you also add a whatsnew entry to v0.25.0.rst?

Sure! Done~

I am puzzled of the purpose of this PR. We don't' allow any comparison of categorical except for eq / ne for non-ordered. I would be ok with simply raising the same TypeError message that we already do. But what is the purpose of this expansion?

Just want to get some clarification:
Scenario 1: Missing values.

In [50]:
cat = pd.Categorical([1, 2, 3, None], categories=[1, 2, 3], ordered=True) 
cat
Out[50]:
[1, 2, 3, NaN]
Categories (3, int64): [1 < 2 < 3]
In [51]: 
cat <=2
Out[51]:
array([ True,  True, False,  True])

If we allow missing values in the array, then the result should not be raising errors.
People suggest the result in Out[51] should be array([ True, True, False, False])

Scenario 2: Values that don't fall into a category as WillAyd suggested.

In [54]:
cat2 = pd.Categorical([1, 2, 3, 999], categories=[1, 2, 3], ordered=True) 
cat2
Out[54]:
[1, 2, 3, NaN]
Categories (3, int64): [1 < 2 < 3]
In [55]:
cat2<=2
Out[55]:
array([ True,  True, False,  True])

Currently it doesn't raise TypeError.

If you want to raise TypeError for both scenarios, I can change the behavior of the comparison with scalar. Let me know your suggestions. Thanks.

if the above scenarios compare with a listlike they raise i believe
a scalar should not be different

if the above scenarios compare with a listlike they raise i believe
a scalar should not be different

I tested comparison with listlike, and it didn't raise TypeError. It only takes comparison with None as False.

cat1 = pd.Categorical([1,2,3,999], categories=[1,2,3], ordered=True)
cat2 = pd.Categorical([2,2,2,2], categories=[1,2,3], ordered=True)
cat1
Out[]:
[1, 2, 3, NaN]
Categories (3, int64): [1 < 2 < 3]
In[]:
cat1 >= cat2
Out[]:
array([False,  True,  True, False])

In this case, do we need to change the behavior of comparison to listlike? So in both cases, the comparison to None will raise Error.

I am puzzled of the purpose of this PR.

The purpose is quite clear to me: fixing a bug. Which is clearly described in the issue: #26504

The bug is that when doing a comparison (which is allowed for ordered categoricals), missing values always evaluate to True or False (depending on which operations, because they are encoded as -1 and we use that value in the comparison).
While comparisons to NaN should always be False.

To show this in another way:

In [49]: cat = pd.Categorical([1, 2, 3, None], categories=[1, 2, 3], ordered=True)

In [50]: pd.Series(cat)           
Out[50]: 
0      1
1      2
2      3
3    NaN
dtype: category
Categories (3, int64): [1 < 2 < 3]

In [51]: pd.Series(cat) < 2    
Out[51]: 
0     True
1    False
2    False
3     True
dtype: bool

In [52]: pd.Series(cat, dtype=object) < 2                     
Out[52]: 
0     True
1    False
2    False
3    False
dtype: bool

The object dtype has the correct behaviour: a missing value gives False in the result.

So to be clear this has nothing to do with the None value right?

This is not about "None", or also not about values that don't fall within the category, but is is about missing values (but both None as non-existent categories when constructing the categorical lead to missing values in the actual categorical)

this needs to raise for non ordered categoricals

that is not addressed in this PR as there are no tests for this

furthermore the behavior for listlikes must also be the same (raise for non ordered)
if u r making work for scalars then it needs to work for listlikes as well (for ordered)

this needs to raise for non ordered categoricals

that is not addressed in this PR as there are no tests for this

I think the issue was about ordered Categoricals. unordered does indeed raise

In [3]: import pandas as pd
   ...: pd.Categorical(["1", "2", "3", None], categories=["1", "2", "3"]) <= "2"
   ...:
   ...:
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-3-1804d8ae703c> in <module>
      1 import pandas as pd
----> 2 pd.Categorical(["1", "2", "3", None], categories=["1", "2", "3"]) <= "2"
      3

~/sandbox/pandas/pandas/core/arrays/categorical.py in f(self, other)
     64         if not self.ordered:
     65             if op in ['__lt__', '__gt__', '__le__', '__ge__']:
---> 66                 raise TypeError("Unordered Categoricals can only compare "
     67                                 "equality or not")
     68         if isinstance(other, Categorical):

TypeError: Unordered Categoricals can only compare equality or not

this needs to raise for non ordered categoricals

This does already raise, that is not the issue.

furthermore the behavior for listlikes must also be the same. if u r making work for scalars then it needs to work for listlikes as well (for ordered)

This does already work for listlikes. Actually this PR basically copied the code from the code branch for list-likes to the code branch of scalars.

This is not about "None", or also not about values that don't fall within the category, but is is about missing values (but both None as non-existent categories when constructing the categorical lead to missing values in the actual categorical)

Gotcha - so in both in the original issue and here in the whatsnew we keep referring to None as being of importance but that is distracting from the actual issue.

@yanglinlee looks like you might have some other changes to the whatsnew but can you account for that on next edit? Ping back if you are stuck on wording

Gotcha - so in both in the original issue and here in the whatsnew we keep referring to None as being of importance but that is distracting from the actual issue.

Yes, it was by using "None" instead of "missing values" that caused some confusion I think. I already changed the title of this PR.

Thank everyone for the helpful discussions! This is great suggestion! I will update the PR after work tonight.

@yanglinlee CI is being fixed in #26586. Merge master & repush once that's in.

thanks @yanglinlee